fix: MeaiFunction ignoring primitive data types arguments #28
Conversation
…vidual methods to Tools.
…emaAttribute.cs and FunctionToolAttribute.cs
# Conflicts: # src/tests/CSharpToJsonSchema.AotTests/JsonSerializationTests.cs
Replaced direct dictionary access with TryGetValue to prevent potential KeyNotFound exceptions. This ensures safer and more robust handling of missing keys when fetching "mainFunction_Desc".
Removed unnecessary try-catch block around async call invocation and adjusted formatting for better code clarity. Minor whitespace and indentation improvements were also made to enhance consistency and maintainability.
Added detailed XML documentation for MeaiFunction class and its members, improving code readability and maintainability. Introduced support for strict mode and updated method implementations to enhance functionality. These changes provide better structure and guidance for future development.
Replaced GenericFunctionTool with GoogleFunctionTool to align with the updated namespace and class structure. This ensures compatibility with the latest library changes and maintains consistency in functionality.
# Conflicts: # CSharpToJsonSchema.sln # src/libs/CSharpToJsonSchema/MeaiFunction.cs
Generated snapshot files for ToolsJsonSerializerContext to support integration tests for various tool methods and their serialization. This includes handling for different types of arguments like string, void, and async inputs.
The `MethodFunction` test method is commented out, likely due to being unused or obsolete. This change improves code clarity by removing an inactive test without deleting it entirely, preserving it for potential future use.
# Conflicts: # CSharpToJsonSchema.sln # src/tests/AotConsole/Services/StudentRecordService.cs
refactor: Simplified JsonSchema generation codes
WalkthroughThis pull request adds a new project to the solution and updates the core JSON schema conversion logic while streamlining test infrastructure. In the main libraries, the changes remove the use of the parameter descriptions in the conversion flow, update initialization of method descriptions, and add a new conditional branch for handling extra JSON element types. The schema conversion method now accepts a different signature and uses helper methods for description extraction and type fixing. In addition, a large number of auto-generated snapshot and JSON serialization files have been removed from the tests. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Caller
participant SB as SchemaBuilder
participant ED as ExtractDescription
participant FT as FixType
C->>SB: Call ConvertToSchema(JsonTypeInfo, descriptionString)
SB->>SB: Invoke GetJsonSchemaAsNode
SB->>ED: Call ExtractDescription(context, schema, dics)
ED-->>SB: Return updated schema node
SB->>FT: Call FixType(schema)
FT-->>SB: Return fixed schema node
SB-->>C: Return OpenApiSchema
sequenceDiagram
participant M as MeaiFunction
participant JE as JsonElement
M->>M: Evaluate args.Value type
alt JsonElement is array or object
M->>M: Process using existing branches
else
M->>M: Create JsonValue using JsonValue.Create(element)
M->>M: Assign value to jsonObject[args.Key]
end
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Gunpal Jain seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
CSharpToJsonSchema.sln (1)
85-87:⚠️ Potential issueConfiguration issue in the solution file
The pipeline is reporting warnings about missing Release configuration for the new projects.
{DC07C90E-A58C-44B3-82D2-E2EB8F777B92}.Debug|Any CPU.ActiveCfg = Debug|Any CPU {DC07C90E-A58C-44B3-82D2-E2EB8F777B92}.Debug|Any CPU.Build.0 = Debug|Any CPU +{DC07C90E-A58C-44B3-82D2-E2EB8F777B92}.Release|Any CPU.ActiveCfg = Release|Any CPU +{DC07C90E-A58C-44B3-82D2-E2EB8F777B92}.Release|Any CPU.Build.0 = Release|Any CPU +{1942E3E5-F151-4C90-BECE-140AAD8C66DE}.Release|Any CPU.ActiveCfg = Release|Any CPU +{1942E3E5-F151-4C90-BECE-140AAD8C66DE}.Release|Any CPU.Build.0 = Release|Any CPU
🧹 Nitpick comments (7)
src/libs/CSharpToJsonSchema/MeaiFunction.cs (1)
94-96: Empty block for JsonNode handlingThe code has an empty block for handling JsonNode arguments which could potentially be improved.
Consider implementing proper handling for JsonNode arguments instead of leaving an empty block:
else if (args.Value is JsonNode node) { + jsonObject[args.Key] = node; }src/tests/CSharpToJsonSchema.AotTests/JsonSerializationTests.cs (2)
130-132: Remove duplicate test fortool.Description.Lines 131 and 135 both assert
tool.Description.Should().Be("Get student record for the year");. Repeating the same assertion is redundant unless there's a very specific reason to validate the same condition twice.130 Tool tool = tools[0]; -131 tool.Description.Should().Be("Get student record for the year"); 132
134-138: Refinement suggestion for improved readability.The lines here add clarity to retrieving and casting
tool.Parametersto anOpenApiSchema. The usage looks valid; however, consider grouping the schema reference and its asserts together for readability. For instance:
- Take note that line 135 is a duplication of line 131.
- Put the object creation/cast immediately above or below the
toolassert lines to keep test arrangements clear.src/libs/CSharpToJsonSchema/SchemaSubsetHelper.cs (4)
20-36: Simplify return statements inTransformSchemaNode.Currently, lines 32-35 always return
schemawhethercontext.PropertyInfois null or not. You could move thereturn schema;outside theifblock to remove repetition, making the code more concise.+ TransformSchemaNode = (context, schema) => { if (context.TypeInfo.Type.IsEnum) { schema["type"] = "string"; } - if (context.PropertyInfo == null) - return schema; ExtractDescription(context, schema, dics); - return schema; + return schema; },
40-44: Validate assumption that all properties are required.Auto-adding all properties to
requiredmight be too broad if your JSON schema is meant to allow optional fields. Clarify whether this is intended behavior or consider differentiating required vs. optional.
45-45: Replace Magic String “mainFunction_Desc”.Using “mainFunction_Desc” inline might be fragile. Consider introducing a well-named constant or providing an explanatory comment so future maintainers understand why “mainFunction_Desc” is special.
99-125: Fix typo in error message and verify type array safety.
- Typo: “strucutured” should be “structured” in the message describing Google’s API requirement.
- Consider gracefully handling cases when array elements are not strings, or containing unexpected data types, rather than unconditionally calling
GetValue<string>().- $"Google's API for strucutured output requires every property to have one defined type, ... + $"Google's API for structured output requires every property to have one defined type, ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (50)
CSharpToJsonSchema.sln(1 hunks)src/libs/CSharpToJsonSchema.Generators/Conversion/ToModels.cs(0 hunks)src/libs/CSharpToJsonSchema.Generators/Models/MethodData.cs(0 hunks)src/libs/CSharpToJsonSchema.Generators/Sources.Tools.cs(1 hunks)src/libs/CSharpToJsonSchema/MeaiFunction.cs(1 hunks)src/libs/CSharpToJsonSchema/SchemaSubsetHelper.cs(2 hunks)src/tests/AotConsole/Program.cs(1 hunks)src/tests/CSharpToJsonSchema.AotTests/JsonSerializationTests.cs(1 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.Boolean.g.verified.cs(0 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.Calls.generated.verified.cs(0 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.DateOnly.g.verified.cs(0 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.DateTime.g.verified.cs(0 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.Double.g.verified.cs(0 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.GetCurrentWeather3Args.g.verified.cs(0 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.GetJsonTypeInfo.g.verified.cs(0 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.GetValueArgs.g.verified.cs(0 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.GetValueAsyncArgs.g.verified.cs(0 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.Int32.g.verified.cs(0 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.Int64.g.verified.cs(0 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.PropertyNames.g.verified.cs(0 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.SetValueArgs.g.verified.cs(0 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.SetValueAsyncArgs.g.verified.cs(0 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.Single.g.verified.cs(0 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.Tools.generated.verified.cs(0 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.g.verified.cs(0 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes_Diagnostics.verified.txt(0 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather#IWeatherTools.Calls.generated.verified.cs(0 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather#IWeatherTools.Double.g.verified.cs(0 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather#IWeatherTools.GetCurrentWeatherArgs.g.verified.cs(0 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather#IWeatherTools.GetCurrentWeatherAsyncArgs.g.verified.cs(0 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather#IWeatherTools.GetJsonTypeInfo.g.verified.cs(0 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather#IWeatherTools.PropertyNames.g.verified.cs(0 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather#IWeatherTools.String.g.verified.cs(0 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather#IWeatherTools.Tools.generated.verified.cs(0 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather#IWeatherTools.Unit.g.verified.cs(0 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather#IWeatherTools.Weather.g.verified.cs(0 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather#IWeatherTools.g.verified.cs(0 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.WeatherStrict#IWeatherStrictTools.Calls.generated.verified.cs(0 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.WeatherStrict#IWeatherStrictTools.Double.g.verified.cs(0 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.WeatherStrict#IWeatherStrictTools.GetCurrentWeather2Args.g.verified.cs(0 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.WeatherStrict#IWeatherStrictTools.GetCurrentWeatherAsync2Args.g.verified.cs(0 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.WeatherStrict#IWeatherStrictTools.GetJsonTypeInfo.g.verified.cs(0 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.WeatherStrict#IWeatherStrictTools.PropertyNames.g.verified.cs(0 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.WeatherStrict#IWeatherStrictTools.String.g.verified.cs(0 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.WeatherStrict#IWeatherStrictTools.Tools.generated.verified.cs(0 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.WeatherStrict#IWeatherStrictTools.Unit2.g.verified.cs(0 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.WeatherStrict#IWeatherStrictTools.Weather2.g.verified.cs(0 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.WeatherStrict#IWeatherStrictTools.g.received.cs(1 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.WeatherStrict_Diagnostics.verified.txt(0 hunks)src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather_Diagnostics.verified.txt(0 hunks)
💤 Files with no reviewable changes (43)
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes_Diagnostics.verified.txt
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.WeatherStrict#IWeatherStrictTools.PropertyNames.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather_Diagnostics.verified.txt
- src/libs/CSharpToJsonSchema.Generators/Conversion/ToModels.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.GetJsonTypeInfo.g.verified.cs
- src/libs/CSharpToJsonSchema.Generators/Models/MethodData.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.Boolean.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.WeatherStrict_Diagnostics.verified.txt
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.WeatherStrict#IWeatherStrictTools.GetJsonTypeInfo.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.WeatherStrict#IWeatherStrictTools.Unit2.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather#IWeatherTools.PropertyNames.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather#IWeatherTools.GetJsonTypeInfo.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather#IWeatherTools.Unit.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather#IWeatherTools.Tools.generated.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.PropertyNames.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.SetValueArgs.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.WeatherStrict#IWeatherStrictTools.Tools.generated.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather#IWeatherTools.Weather.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.Tools.generated.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.WeatherStrict#IWeatherStrictTools.Double.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.Double.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.DateOnly.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather#IWeatherTools.Double.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.WeatherStrict#IWeatherStrictTools.Weather2.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.Int32.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.Single.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.SetValueAsyncArgs.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather#IWeatherTools.Calls.generated.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.Int64.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.DateTime.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather#IWeatherTools.GetCurrentWeatherArgs.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.WeatherStrict#IWeatherStrictTools.GetCurrentWeatherAsync2Args.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather#IWeatherTools.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather#IWeatherTools.String.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.WeatherStrict#IWeatherStrictTools.Calls.generated.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.GetValueAsyncArgs.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.WeatherStrict#IWeatherStrictTools.String.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.Weather#IWeatherTools.GetCurrentWeatherAsyncArgs.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.WeatherStrict#IWeatherStrictTools.GetCurrentWeather2Args.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.GetValueArgs.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.GetCurrentWeather3Args.g.verified.cs
- src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.VariousTypes#IVariousTypesTools.Calls.generated.verified.cs
🧰 Additional context used
🪛 GitHub Actions: Test
CSharpToJsonSchema.sln
[warning] 1-1: The project configuration for project 'CSharpToJsonSchema.MeaiTests' was not specified in the solution file for the solution configuration 'Release|Any CPU'.
[warning] 1-1: The project configuration for project 'AotConsole' was not specified in the solution file for the solution configuration 'Release|Any CPU'.
🔇 Additional comments (11)
src/tests/CSharpToJsonSchema.SnapshotTests/Snapshots/ToolTests.WeatherStrict#IWeatherStrictTools.g.received.cs (1)
12-12: Note on Version Number Update in GeneratedCode Attribute
The version number in theGeneratedCodeAttributehas been updated from"3.0.0.0"to"0.0.0.0". This change appears to be part of the restructuring effort. Please verify that this new version metadata is intentional and consistent with your generator’s versioning scheme, ensuring that any tooling or consumers relying on this metadata will not be negatively impacted.src/libs/CSharpToJsonSchema.Generators/Sources.Tools.cs (1)
100-101: Refactored method description initialization approachThis change simplifies the description handling by using a single description value instead of relying on a collection of descriptions. This aligns with the removal of the
Descriptionsproperty from theMethodDatarecord struct as mentioned in the AI summary.src/tests/AotConsole/Program.cs (1)
12-12: Commented out unnecessary test promptRemoving this specific test prompt cleans up the code while maintaining the primary functionality for testing with the student record prompt.
CSharpToJsonSchema.sln (1)
45-46: Added new MeaiTests project to the solutionThe addition of this specialized test project is appropriate for validating the changes to the MeaiFunction implementation.
src/libs/CSharpToJsonSchema/MeaiFunction.cs (1)
91-92: Fixed handling of primitive JSON typesThis change addresses the core issue mentioned in the PR title. Previously, the MeaiFunction was ignoring primitive data types in arguments because it only handled arrays and objects. Adding this else branch ensures that all primitive types (strings, numbers, booleans, etc.) are properly converted to JsonValue objects.
The implementation is correct as it uses
JsonValue.Create()which handles all primitive JSON types appropriately.src/tests/CSharpToJsonSchema.AotTests/JsonSerializationTests.cs (5)
139-141: No issues detected.Validating the
fullNameproperty as a string with a descriptive text is appropriate. Looks good as written.
142-146: Proper schema validation forgradeFilters.Ensures the property is declared as an array of strings with an appropriate description. Logic and naming are consistent with best practices for JSON-based schemas.
147-151: Correct usage of date-time format.Declaring a property type as
"string"with"date-time"format makes sense forenrollmentStartDateand the description is well-specified.
152-156: Consistent date-time schema approach.This mirrors the logic above for
enrollmentEndDate, ensuring the same type and format usage. Implementation is coherent.
157-159: Descriptive boolean field.Marking
isActiveas a boolean and providing a clear description is aligned with JSON schema standards. No issues found.src/libs/CSharpToJsonSchema/SchemaSubsetHelper.cs (1)
13-14: Consider handling null or invaliddescriptionString.
JsonSerializer.Deserializecall on line 17 may throw an exception ifdescriptionStringis null or invalid JSON. Ensure upstream callers or this method itself gracefully handles those edge cases.
| private static void ExtractDescription(JsonSchemaExporterContext context, JsonNode schema, IDictionary<string, string> dics) | ||
| { | ||
| // Determine if a type or property and extract the relevant attribute provider. | ||
| ICustomAttributeProvider? attributeProvider = context.PropertyInfo is not null | ||
| ? context.PropertyInfo.AttributeProvider | ||
| : context.TypeInfo.Type; | ||
|
|
||
| // Look up any description attributes. | ||
| DescriptionAttribute? descriptionAttr = attributeProvider? | ||
| .GetCustomAttributes(inherit: true) | ||
| .Select(attr => attr as DescriptionAttribute) | ||
| .FirstOrDefault(attr => attr is not null); | ||
|
|
||
| var description = descriptionAttr?.Description; | ||
| if (string.IsNullOrEmpty(description)) | ||
| { | ||
| if (context.PropertyInfo is null) | ||
| { | ||
| var propertyName = ToCamelCase(context.TypeInfo.Type.Name); | ||
| dics.TryGetValue(propertyName, out description); | ||
| } | ||
| } | ||
|
|
||
| FixType(schema); | ||
|
|
||
| // Apply description attribute to the generated schema. | ||
| if (description is not null) | ||
| { | ||
| if (schema is not JsonObject jObj) | ||
| { | ||
| // Handle the case where the schema is a Boolean. | ||
| JsonValueKind valueKind = schema.GetValueKind(); | ||
|
|
||
| schema = jObj = new JsonObject(); | ||
| if (valueKind is JsonValueKind.False) | ||
| { | ||
| jObj.Add("not", true); | ||
| } | ||
| } | ||
|
|
||
| jObj.Insert(0, "description", description); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check for possible null references when handling schema objects.
Inside ExtractDescription, if schema is unexpectedly null or not a JsonObject, lines 85–90 do some transformations but assume schema can always be re-cast to JsonObject. Consider explicitly verifying or throwing an exception earlier if schema is null.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/libs/CSharpToJsonSchema/SchemaSubsetHelper.cs (1)
115-117: Inconsistent API reference in error messages.The error messages still reference "LLM's API" while the AI summary suggested changing to "Google's API". Consider updating these error messages for consistency.
- $"LLM's API for strucutured output requires every property to have one defined type, not multiple options. Path: {schema.GetPath()} Schema: {schema.ToJsonString()}"); + $"Google's API for structured output requires every property to have one defined type, not multiple options. Path: {schema.GetPath()} Schema: {schema.ToJsonString()}");Also note the typo "strucutured" which should be "structured".
Also applies to: 121-123
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/libs/CSharpToJsonSchema.Generators/Conversion/ToModels.cs(0 hunks)src/libs/CSharpToJsonSchema/SchemaSubsetHelper.cs(2 hunks)
💤 Files with no reviewable changes (1)
- src/libs/CSharpToJsonSchema.Generators/Conversion/ToModels.cs
🔇 Additional comments (5)
src/libs/CSharpToJsonSchema/SchemaSubsetHelper.cs (5)
12-53: Updated schema conversion approach improves type handling.The revised
ConvertToSchemamethod now directly usesGetJsonSchemaAsNodewith a custom transformation function, which simplifies the code flow. The schema transformation is now broken into smaller, more focused operations that seem to better handle various type scenarios.
20-36: Simplified schema node transformation.The transformation process has been refactored to separate concerns. Enum handling converts enums to strings, and description extraction has been moved to a dedicated method. This separation of concerns makes the code more maintainable.
55-97: Improved description extraction and null handling.The new
ExtractDescriptionmethod properly addresses the previous review comment by explicitly handling the case where the schema is not a JsonObject. The code now properly transforms boolean schemas to objects before adding the description property.
83-93: Null-safe handling of schema objects.This section properly addresses the previous review comment about null references when handling schema objects. The code now correctly handles the case where the schema is a boolean value rather than an object by creating a new JsonObject and setting appropriate values.
99-125: FixType method addresses the primitive type handling issue.This new method appears to be the key implementation for fixing the MeaiFunction primitive data types issue mentioned in the PR objectives. It properly handles cases where "type" is an array (which can occur with nullable types or union types) and collapses it to a single type with nullable flag when appropriate.
3 participants
Fixed MeaiFunction tool was ignoring primitive data types arguments
Refactor and simplified Json Schema generation codes.
Summary by CodeRabbit
Descriptionsproperty fromMethodData, altering the structure of method data processing.JsonElementtypes in theGetArgsStringmethod.OpenApiSchemarelated to tools.